Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: remove global network usage from transaction confirmations #28236

Merged
merged 13 commits into from
Nov 8, 2024
Merged

refactor: remove global network usage from transaction confirmations #28236

merged 13 commits into from
Nov 8, 2024

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Nov 1, 2024 •
edited
Loading

Description

Remove usages of global network selectors from transaction confirmation React components and hooks.

Specifically:

  • Remove usages of the following selectors:
    • getConversionRate
    • getCurrentChainId
    • getNativeCurrency
    • getNetworkIdentifier
    • getNftContracts
    • getNfts
    • getProviderConfig
    • getRpcPrefsForCurrentProvider
  • Add new selectors:
    • selectConversionRateByChainId
    • selectNftsByChainId
    • selectNftContractsByChainId
    • selectNetworkIdentifierByChainId
  • Add ESLint rule to prevent further usage of global network selectors in confirmations directory.

Open in GitHub Codespaces

Related issues

Fixes: #3469 #3373 #3486 #3487

Manual testing steps

Full regression of all transaction confirmations and related functionality.

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@github-actions github-actions bot added the team-confirmations Push issues to confirmations team label Nov 1, 2024
@matthewwalsh0 matthewwalsh0 changed the base branch from develop to refactor/remove-global-network-usage-signatures November 1, 2024 20:48
Base automatically changed from refactor/remove-global-network-usage-signatures to develop November 4, 2024 10:12
@matthewwalsh0 matthewwalsh0 force-pushed the refactor/remove-global-network-usage-transactions branch from 1a2436d to 4751ebf Compare November 5, 2024 12:24
@metamaskbot
Copy link
Collaborator

Builds ready [4751ebf]
Page Load Metrics (2019 ± 105 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30627461922427205
domContentLoaded170227351995220105
load174927472019219105
domInteractive179647199
backgroundConnect106424168
firstReactRender572091253919
getState566282512
initialActions01000
loadScripts11652014147118086
setupStore1197402914
uiStartup196430012313265127
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 4 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [64e464e]
Page Load Metrics (1940 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint23022851874410197
domContentLoaded16882214191715775
load16922230194016278
domInteractive23204573919
backgroundConnect778232110
firstReactRender482951256732
getState56414168
initialActions01000
loadScripts11581636137513464
setupStore1192312713
uiStartup191127402213224107
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: -274 Bytes (-0.00%)
  • common: 244 Bytes (0.00%)

@matthewwalsh0 matthewwalsh0 changed the title refactor: remove global network usage from transactions refactor: remove global network usage from transaction confirmations Nov 5, 2024
@matthewwalsh0 matthewwalsh0 marked this pull request as ready for review November 5, 2024 16:31
@matthewwalsh0 matthewwalsh0 requested review from a team as code owners November 5, 2024 16:31
@metamaskbot
Copy link
Collaborator

Builds ready [43317f9]
Page Load Metrics (2003 ± 172 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27233671924518249
domContentLoaded170832961974347166
load171833692003358172
domInteractive2188512210
backgroundConnect777242211
firstReactRender532901164522
getState46417199
initialActions01000
loadScripts122623621454261125
setupStore1180342311
uiStartup194136152254361173
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: -29 Bytes (-0.00%)
  • common: 883 Bytes (0.01%)

@sleepytanya
Copy link
Contributor

sleepytanya commented Nov 6, 2024 •
edited
Loading

STX toggle is ON but the transaction is submitted as regular Send, user is able to customize nonce qand send the transaction with the low / high nonce value. On the latest develop STX work as expected.

regularTx.mov

Apart from STX, all other functionalities seem to be operating as anticipated.

@metamaskbot
Copy link
Collaborator

Builds ready [f8c33c4]
Page Load Metrics (2079 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25025571921542260
domContentLoaded18382538203918187
load18512558207918991
domInteractive2198512211
backgroundConnect8134423316
firstReactRender531961073517
getState778292512
initialActions01000
loadScripts12761866148215574
setupStore1272362311
uiStartup207429452341236113
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: -147 Bytes (-0.00%)
  • common: 883 Bytes (0.01%)

@sleepytanya
Copy link
Contributor

STX function as expected!

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Collaborator

Builds ready [1bf2dac]
Page Load Metrics (1867 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint65423761809318153
domContentLoaded16482214184015172
load16572294186716177
domInteractive2298552010
backgroundConnect999242311
firstReactRender522931144924
getState5259295627
initialActions01000
loadScripts11461615134712459
setupStore594172311
uiStartup185329292113264127
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: -147 Bytes (-0.00%)
  • common: 883 Bytes (0.01%)

@matthewwalsh0 matthewwalsh0 added this pull request to the merge queue Nov 8, 2024
Merged via the queue into develop with commit c564383 Nov 8, 2024
76 checks passed
@matthewwalsh0 matthewwalsh0 deleted the refactor/remove-global-network-usage-transactions branch November 8, 2024 10:48
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pedronfigueiredo pedronfigueiredo pedronfigueiredo approved these changes

@jpuri jpuri jpuri approved these changes

@OGPoyraz OGPoyraz Awaiting requested review from OGPoyraz

@vinistevam vinistevam Awaiting requested review from vinistevam

Assignees
No one assigned
Labels
QA Passed release-12.8.0 Issue or pull request that will be included in release 12.8.0 team-confirmations Push issues to confirmations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matthewwalsh0 @metamaskbot @sleepytanya @jpuri @pedronfigueiredo